Rust: SQL Injection Query #18025

geoffw0 · 2024-11-19T11:05:51Z

Adds a query rust/sql-injection to detect SQL injection vulnerabilities in Rust. Note that there's a query, docs, tests, and various wiring for models (including a Concepts.qll) but no actual source or sink models are implemented in this PR - so no results will be found at this time. Nevertheless I'm keen to get reviewers eyes on this and get something merged while I work on those models as a follow-up.

The test is quite slow, due to fetching dependencies. We will want the stub generator before we have a large query suite IMO.

TODO:

code review
docs review
DCA run (there won't be any results)

I'm deferring discussions about autofix until we have sources + sinks and thus results to evaluate.

…') so that the query test can function.

github-actions · 2024-11-19T11:07:00Z

QHelp previews:

rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp

Database query built from user-controlled sources

If a database query (such as an SQL query) is built from user-provided data without sufficient sanitization, a user may be able to run malicious database queries. An attacker can craft the part of the query they control to change the overall meaning of the query.

Recommendation

Most database connector libraries offer a way to safely embed untrusted data into a query using query parameters or prepared statements. You should use these features to build queries, rather than string concatenation or similar methods. You can also escape (sanitize) user-controlled strings so that they can be included directly in an SQL command. A library function should be used for escaping, because this approach is only safe if the escaping function is robust against all possible inputs.

Example

In the following examples, an SQL query is prepared using string formatting to directly include a user-controlled value remote_controlled_string. An attacker could craft remote_controlled_string to change the overall meaning of the SQL query.

// with SQLx

let unsafe_query = format!("SELECT * FROM people WHERE firstname='{remote_controlled_string}'");

let _ = conn.execute(unsafe_query.as_str()).await?; // BAD (arbitrary SQL injection is possible)

let _ = sqlx::query(unsafe_query.as_str()).fetch_all(&mut conn).await?; // BAD (arbitrary SQL injection is possible)

A better way to do this is with a prepared statement, binding remote_controlled_string to a parameter of that statement. An attacker who controls remote_controlled_string now cannot change the overall meaning of the query.

// with SQLx

let prepared_query = "SELECT * FROM people WHERE firstname=?";

let _ = sqlx::query(prepared_query_1).bind(&remote_controlled_string).fetch_all(&mut conn).await?; // GOOD (prepared statement with bound parameter)

References

Wikipedia: SQL injection.
OWASP: SQL Injection Prevention Cheat Sheet.
Common Weakness Enumeration: CWE-89.

…of this is framework, nothing is concretely modelled yet.

rust/ql/test/query-tests/security/CWE-089/sqlx.rs

rust/ql/src/queries/security/CWE-089/SqlInjection.ql

sunbrye

From the docs team—did an editorial review and this looks great! I'll let someone else perform the technical review of this though

hvitved · 2024-11-20T10:26:28Z

rust/ql/lib/codeql/rust/Concepts.qll

+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `ThreatModelSource::Range` instead.
+ */
+class ThreatModelSource extends DataFlow::Node instanceof ThreatModelSource::Range {


Can't this simply be final class ThreatModelSource = ThreatModelSource::Range?

I haven't worked with Range classes before. My understanding is that in the frameworks we're expected to sometimes want to override both, so this can't be final, but .... I will admit I'm not really sure what the point of a separate class and Range is.

I don't see why one would want to override both ThreatModelSource and ThreatModelSource::Range. I think we should change it like above.

OK, I think I see what's going on here. The intent of the "Range" pattern is that you can extend the abstract Range class to expand the class to cover new things (i.e. in this case to add new sources). But if you want to define something in terms of ThreatModelSource (i.e. extend it the non-abstract way, as ActiveThreatModelSource does), you use the non-Range version. Making it final actually doesn't remove the latter possibility, thanks to "final extensions" (which I haven't knowingly used much before).

Change made. The code is quite a lot cleaner as a result. Other languages could implement the same change in their Concepts.qll.

rust/ql/lib/codeql/rust/Concepts.qll

hvitved · 2024-11-20T10:31:51Z

rust/ql/lib/codeql/rust/security/SqlInjectionExtensions.qll

+  /**
+   * An active threat-model source, considered as a flow source.
+   */
+  private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }


Shouldn't this be restricted somehow using getSourceType()?

I don't think so. ActiveThreatModelSource is a restriction of ThreatModelSource according to user preferences (however that works), which defaults to just the remote sources I think. This sounds like exactly what we want.

Ah ok, to ActiveThreatModelSource should be used for all queries that already use remote flow sources?

I believe that's the idea. Unless you have a special reason for really only wanting remote flow sources, but generally in the past we've generally specified remote only because most users aren't interested in flow from argv

hvitved · 2024-11-20T10:32:57Z

rust/ql/src/queries/security/CWE-089/SqlInjection.ql

+/**
+ * A taint configuration for tainted data that reaches a SQL sink.
+ */


I think this comment is redundant.

As in, it doesn't say much that isn't obvious from the code below it?

I tend to comment nearly every file, class and module (with occasional exceptions). One reason is I think it's a requirement in .qlls. Another is that I personally tend to read comments before I look at code, so a module without a comment is harder to read...

rust/ql/src/queries/security/CWE-089/SqlInjection.ql

rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs

geoffw0 · 2024-11-21T14:08:09Z

rust/ql/lib/codeql/rust/Concepts.qll

+  /**
+   * A data flow source, for a specific threat-model.
+   */
+  abstract class Range extends DataFlow::Node {


I'd be happy to do away with the doc on the ThreadModelSource module and ThreadModelSource::Range class, as it really just duplicates that on ThreadModelSource. I don't think CI would accept that though???

mchammer01 · 2024-11-21T15:48:07Z

I'll review this for Docs, either later today, or tomorrow.

mchammer01

LGTM, approving to unblock ✨
Made some minor comments, that you can ignore if you wish.
Sorry for re-reviewing this @sunbrye but it was still unassigned in our security products board, and I only realized now that you'd already given this an editorial review 😞

rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp

rust/ql/src/queries/security/CWE-089/SqlInjection.ql

Co-authored-by: mc <[email protected]>

geoffw0 · 2024-11-21T17:10:16Z

@hvitved are you happy with the changes to this PR?

geoffw0 · 2024-11-21T22:49:33Z

Merged. But I'm still working on follow-up improvements to this query, so any late comments are still welcome!

geoffw0 added 3 commits November 19, 2024 10:56

Rust: Add SQL injection test cases (complete and functioning).

3673c7c

Rust: Add placeholder SQL injection query, linked in test.

28d0ad9

Rust: Add options.yml and sqlx 'query cache' (result of 'sqlx prepare…

2df565c

…') so that the query test can function.

geoffw0 added no-change-note-required This PR does not need a change note ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. Rust Pull requests that update Rust code labels Nov 19, 2024

geoffw0 requested a review from a team as a code owner November 19, 2024 11:05

github-actions bot added documentation Swift labels Nov 19, 2024

geoffw0 added 2 commits November 19, 2024 11:09

Rust: Add .qhelp and examples.

6a7fb06

Rust: Implement query, source/sink/barrier classes and concepts. All …

c7c6924

…of this is framework, nothing is concretely modelled yet.

github-advanced-security bot found potential problems Nov 19, 2024

View reviewed changes

rust/ql/test/query-tests/security/CWE-089/sqlx.rs Dismissed Show dismissed Hide dismissed

rust/ql/test/query-tests/security/CWE-089/sqlx.rs Dismissed Show dismissed Hide dismissed

rust/ql/test/query-tests/security/CWE-089/sqlx.rs Dismissed Show dismissed Hide dismissed

geoffw0 force-pushed the sql1 branch from a6f0cb8 to c7c6924 Compare November 19, 2024 11:10

github-actions bot removed the Swift label Nov 19, 2024

github-advanced-security bot found potential problems Nov 19, 2024

View reviewed changes

rust/ql/src/queries/security/CWE-089/SqlInjection.ql Show resolved Hide resolved

geoffw0 added 2 commits November 19, 2024 11:49

Rust: Autoformat.

49eefcc

Rust: Add consistency check failures.

758092b

sunbrye previously approved these changes Nov 19, 2024

View reviewed changes

hvitved reviewed Nov 20, 2024

View reviewed changes

Rust: Address review comments.

d828941

geoffw0 dismissed sunbrye’s stale review via d828941 November 20, 2024 22:39

geoffw0 added 2 commits November 21, 2024 13:57

Rust: Use final class.

636c047

Rust: Restore some documentation.

49e0028

geoffw0 commented Nov 21, 2024

View reviewed changes

geoffw0 added 2 commits November 21, 2024 14:18

Merge branch 'main' into sql1

ae6b7b0

Rust: Accept consistency check failures.

b7c7a9e

mchammer01 self-requested a review November 21, 2024 15:47

mchammer01 previously approved these changes Nov 21, 2024

View reviewed changes

rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp Outdated Show resolved Hide resolved

rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp Outdated Show resolved Hide resolved

rust/ql/src/queries/security/CWE-089/SqlInjection.ql Outdated Show resolved Hide resolved

Update rust/ql/src/queries/security/CWE-089/SqlInjection.ql

f8fff4b

Co-authored-by: mc <[email protected]>

geoffw0 dismissed mchammer01’s stale review via f8fff4b November 21, 2024 17:06

Rust: Suggestions from docs review.

01cddcc

hvitved approved these changes Nov 21, 2024

View reviewed changes

geoffw0 merged commit b6cdae2 into github:main Nov 21, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: SQL Injection Query #18025

Rust: SQL Injection Query #18025

geoffw0 commented Nov 19, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024 •

edited

Loading

Database query built from user-controlled sources

Recommendation

Example

References

sunbrye left a comment •

edited

Loading

hvitved Nov 20, 2024

geoffw0 Nov 20, 2024

hvitved Nov 21, 2024

geoffw0 Nov 21, 2024

hvitved Nov 20, 2024

geoffw0 Nov 20, 2024

hvitved Nov 21, 2024

geoffw0 Nov 21, 2024

hvitved Nov 20, 2024

geoffw0 Nov 20, 2024

geoffw0 Nov 21, 2024

mchammer01 commented Nov 21, 2024

mchammer01 left a comment

geoffw0 commented Nov 21, 2024

geoffw0 commented Nov 21, 2024

Rust: SQL Injection Query #18025

Rust: SQL Injection Query #18025

Conversation

geoffw0 commented Nov 19, 2024 • edited Loading

github-actions bot commented Nov 19, 2024 • edited Loading

Database query built from user-controlled sources

Recommendation

Example

References

sunbrye left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchammer01 commented Nov 21, 2024

mchammer01 left a comment

Choose a reason for hiding this comment

geoffw0 commented Nov 21, 2024

geoffw0 commented Nov 21, 2024

geoffw0 commented Nov 19, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024 •

edited

Loading

sunbrye left a comment •

edited

Loading